Skip to content

Conversation

@RinZ27
Copy link
Contributor

@RinZ27 RinZ27 commented Jan 17, 2026

I was looking through the CLI code and spotted a potential security risk when using the mcp dev command on Windows. Because shell=True is required for npx, passing raw arguments can lead to command injection if a user provides a file path containing shell metacharacters.

I decided to use shlex.quote to sanitize these arguments before they are joined into the final command string. This way, I ensure that any special characters are safely escaped, keeping the execution restricted to the intended command. I've verified the fix and it correctly handles paths with spaces and other characters.

@RinZ27 RinZ27 force-pushed the fix/windows-command-injection-v2 branch 2 times, most recently from f7b9f72 to a63bde1 Compare January 20, 2026 14:10
@RinZ27 RinZ27 force-pushed the fix/windows-command-injection-v2 branch from a63bde1 to 4f4dba9 Compare January 23, 2026 11:58
@RinZ27
Copy link
Contributor Author

RinZ27 commented Jan 23, 2026

@Kludex I can't apply it universally because on POSIX I'm using shell=False (passing a raw list), where subprocess handles the arguments directly. If I quoted them there, the quotes would be treated as literal parts of the filename. Manual quoting is only necessary when I'm forced to use shell=True on Windows.

Also, I realized shlex.quote uses single quotes which isn't ideal for cmd.exe. I'll update this to use list2cmdline instead to ensure it works correctly on Windows.

@RinZ27 RinZ27 force-pushed the fix/windows-command-injection-v2 branch 2 times, most recently from ed1565d to 4fd818c Compare January 27, 2026 09:37
Switched from shlex.quote to subprocess.list2cmdline for proper argument escaping on Windows when shell=True is required for npx. This ensures security and reliability when file paths contain special characters.
@RinZ27 RinZ27 force-pushed the fix/windows-command-injection-v2 branch from 95b080c to 93d26b6 Compare January 27, 2026 10:02
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kludex
Copy link
Member

Kludex commented Jan 27, 2026

Also, please don't create PRs with "SECURITY". Security vulnerabilities are supposed to be disclosed via proper means.

@RinZ27
Copy link
Contributor Author

RinZ27 commented Jan 27, 2026

@Kludex Thanks for the clarification and the CPython reference. I see now that subprocess.run already handles the argument quoting internally through list2cmdline, making this manual check redundant. Apologies for the 'SECURITY' tag in the title as well; I'll stick to the official disclosure channels for any future security-related findings. Closing this PR now.

@RinZ27 RinZ27 closed this Jan 27, 2026
@RinZ27 RinZ27 deleted the fix/windows-command-injection-v2 branch January 27, 2026 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants